Skip to content

backup: read board vendor/model from the right cJSON nodes - #182

Open
widgetii wants to merge 1 commit into
masterfrom
fix/upgrade-board-json-accessor
Open

backup: read board vendor/model from the right cJSON nodes#182
widgetii wants to merge 1 commit into
masterfrom
fix/upgrade-board-json-accessor

Conversation

@widgetii

Copy link
Copy Markdown
Member

Problem

do_upgrade() looks up the vendor and model items of the board-info object, then calls cJSON_GetStringValue() on binfo — the object itself — instead of on the item it just found:

cJSON *c_vendor = cJSON_GetObjectItem(binfo, "vendor");
if (c_vendor) {
    char *bstr = cJSON_GetStringValue(binfo);   // <- binfo, not c_vendor
    if (bstr)
        strcpy(board, bstr);
}
cJSON *c_model = cJSON_GetObjectItem(binfo, "model");
if (c_model) {
    char *bstr = cJSON_GetStringValue(binfo);   // <- same
    strcpy(board_id, bstr);                     // <- and unguarded
    ...
}

detect_board() returns an object, and cJSON_GetStringValue() returns NULL for anything that isn't a string, so bstr is NULL in both branches. Consequences:

  • the vendor branch is NULL-guarded, so it silently does nothing;
  • the model branch is not, so it runs strcpy(board_id, NULL).

ipctool upgrade therefore segfaults on any board whose detector supplies a model key — currently Ruision, Anjoy and Hankvision (src/boards/*.c). On boards without one it survives, but board_id stays empty, so the hardware U-Boot variable set from it further down is never written.

Fix

Read from c_vendor/c_model, guard both against NULL, and bound the copies with snprintf — these strings come from board detection (sysinfo files, directory names) and were being strcpy/strcat'ed into fixed 1024-byte buffers unchecked.

Behaviour is otherwise preserved, including board_id carrying the model alone.

Note for the maintainer

board is assembled here and then never read again. I left that as-is rather than quietly change what the composed string is supposed to mean — happy to drop it, or to switch hardware to the full "vendor model" string, if either was the original intent.

Testing

Compile-tested. I haven't run ipctool upgrade on a Ruision/Anjoy/Hankvision board — if someone can reproduce the crash on stock master and confirm it's gone here, that would close the loop.

do_upgrade() looked up the "vendor" and "model" items of the board info
object, then called cJSON_GetStringValue(binfo) on the *object* instead of
on the item it had just found. cJSON_GetStringValue() returns NULL for a
non-string, so:

  - the vendor branch was NULL-guarded and silently did nothing;
  - the model branch was not, and ran strcpy(board_id, NULL).

So `ipctool upgrade` segfaults on any board whose detector supplies a
"model" key — currently Ruision, Anjoy and Hankvision (src/boards/*.c) — and
on every other board it leaves board_id empty, so the "hardware" U-Boot
variable set from it further down never gets written.

Read the values from c_vendor/c_model, guard both against NULL, and bound
the copies with snprintf: the strings come from board detection (sysinfo
files, directory names) and were being strcpy/strcat'ed into fixed 1024-byte
buffers unchecked.

Behaviour is otherwise preserved, including board_id carrying the model
alone.

Note: `board` is assembled here but never read afterwards. That is
pre-existing, so left alone rather than silently changing what the composed
string is meant to be.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@widgetii
widgetii marked this pull request as ready for review July 31, 2026 10:47
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Fix board vendor/model cJSON access in upgrade path

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Fix vendor/model extraction by reading string values from the correct cJSON items.
• Prevent NULL dereference and bound board/board_id writes using snprintf.
• Ensure the U-Boot "hardware" env var is set when a model is provided.
Diagram

graph TD
  A["ipctool upgrade"] --> B["do_upgrade()"] --> C["detect_board()"] --> D["board-info JSON (cJSON)"] --> E["Read vendor/model strings"] --> F["Populate board + board_id"] --> G["Set U-Boot env: hardware"]
Loading
High-Level Assessment

The PR’s approach is the most direct and least disruptive: it fixes the incorrect cJSON_GetStringValue() target, adds the missing NULL guard, and hardens buffer writes with bounded snprintf while preserving existing semantics (board_id remains model-only). Alternatives like switching to explicit cJSON_IsString() + item->valuestring or introducing a shared JSON/string-copy helper were considered but don’t materially improve correctness for this narrowly scoped fix.

Files changed (1) +10 / -12

Bug fix (1) +10 / -12
backup.cFix vendor/model extraction and bound board string writes in do_upgrade() +10/-12

Fix vendor/model extraction and bound board string writes in do_upgrade()

• Corrects vendor/model retrieval by calling cJSON_GetStringValue() on the vendor/model items rather than the board-info object itself. Adds NULL-guards and replaces unbounded strcpy/strcat with snprintf-based bounded writes to prevent crashes and potential buffer overflow, while keeping board_id as the model string and preserving existing behavior.

src/backup.c

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant